Unit tests for step.save and step.skip #197
Conversation
for more information, see https://pre-commit.ci
emolter
added
the
no-changelog-entry-needed
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #197 +/- ##
==========================================
+ Coverage 73.29% 76.94% +3.65%
==========================================
Files 25 25
Lines 1917 1917
==========================================
+ Hits 1405 1475 +70
+ Misses 512 442 -70 ☔ View full report in Codecov by Sentry. |
| return None | ||
|
|
||
|
|
||
| def test_save(tmp_cwd): |
There was a problem hiding this comment.
To check I'm following, does this test, test_skip and test_save_list aim to test the save_results spec feature? This wasn't clear to me when first looking at the test names/setup.
If this is the case would you add docstring and/or rename the tests to document they're testing save_results?
| with open(fname, "w") as f: | ||
| f.write(f"{path}") |
There was a problem hiding this comment.
| with open(fname, "w") as f: | |
| f.write(f"{path}") | |
| with open(fname, "w") as f: | |
| f.write(f"{path}") |
Any reason to not use path as the filename to make the test more representative of a typical step?
| @pytest.mark.xfail( | ||
| reason="Looping over models only works for list and tuple. This should be fixed." | ||
| ) | ||
| def test_save_container(tmp_cwd, model_list): | ||
| """ensure list-like save still works for non-list sequence""" | ||
| container = SimpleContainer(model_list) | ||
| step = StepWithModel() | ||
| step.run(container) | ||
| for i in range(3): | ||
| assert (tmp_cwd / f"test{i}-saved.txt").exists() | ||
|
|
||
|
|
There was a problem hiding this comment.
| @pytest.mark.xfail( | |
| reason="Looping over models only works for list and tuple. This should be fixed." | |
| ) | |
| def test_save_container(tmp_cwd, model_list): | |
| """ensure list-like save still works for non-list sequence""" | |
| container = SimpleContainer(model_list) | |
| step = StepWithModel() | |
| step.run(container) | |
| for i in range(3): | |
| assert (tmp_cwd / f"test{i}-saved.txt").exists() |
My vote is for making an issue/ticket for this instead of adding a known failing test. For the issue perhaps you would add some details about how this fails (does it throw an exception, overwrite files, not save?).
| assert (tmp_cwd / f"test{i}-saved.txt").exists() | ||
|
|
||
|
|
||
| def test_skip_container(tmp_cwd, model_list): |
There was a problem hiding this comment.
To confirm, there are no unit tests that currently test that Step.skip skips a step (for any input)? I'm failing to find any.
This PR adds the unit tests from #190 as it looks like that PR is likely to be withdrawn, at least for the time being, but the unit tests are still relevant to the main branch.
Tasks
docs/pageno-changelog-entry-needed)changes/:echo "changed something" > changes/<PR#>.<changetype>.rst(see below for change types)"git+https://github.com/<fork>/stpipe@<branch>")jwstregression testromancalregression testnews fragment change types...
changes/<PR#>.feature.rst: new featurechanges/<PR#>.bugfix.rst: fixes an issuechanges/<PR#>.doc.rst: documentation changechanges/<PR#>.removal.rst: deprecation or removal of public APIchanges/<PR#>.misc.rst: infrastructure or miscellaneous change